[core] Fix CSndLossList::insert with negative offset #1359
Conversation
maxsharabayko
added
Type: Bug
maxsharabayko
force-pushed
the
hotfix/snd-loss-list-no1000
branch
from
0451d9a
to
4236da8
Compare
maxsharabayko
changed the title
maxsharabayko
force-pushed
the
hotfix/snd-loss-list-no1000
branch
from
4236da8
to
a2b236c
Compare
maxsharabayko
force-pushed
the
hotfix/snd-loss-list-no1000
branch
from
a2b236c
to
34a7134
Compare
srtcore/list.cpp
Outdated
| // The size of the CSndLossList should be at least the size of the flow window. | ||
| // It means that all the packets sender has sent should fit within m_iSize. | ||
| // If the new loss does not fit, there is some error. | ||
| LOGC(mglog.Error, log << "IPE: New loss record is too old. Ignoring. " |
There was a problem hiding this comment.
Not sure if this situation can be described as IPE here.
There was a problem hiding this comment.
BTW. Does it mean that offset is allowed to be negative in general, as long as it fits in the range from the current head and size?
There was a problem hiding this comment.
Potentially the following can happen:
- Two packets with sequence numbers
AandB(A <% B) are reported as lost. - Packet
Ais retransmitted and removed from the loss list. - Packet
Ais reported lost again, so thatCSeqNo::seqoff(m_caSeq[m_iHead].seqstart = B, seqno1 = A) < 0.
So negative offset is allowed, it just has to fit into the list.
There was a problem hiding this comment.
Isn't however head moved forward after getting ACK? Isn't then a sequence that is earlier than head a sequence before the ACK? If this is true, then I can only imagine such a situation if reordering happened and the ACK that confirms the packet in question comes earlier than the lossreport that reports this sequence as lost, although it's actually outdated.
If this is the case, however, then it should be first checked if the upper sequence is below the head, and only if so should the loss record be rejected as a whole. Otherwise at least the range between the head and the upper sequence be recorded.
There was a problem hiding this comment.
The head is moved forward after the loss record is extracted and the corresponding packet is retransmitted.
If the same packet is reported lost again, it is placed before the head (negative offset), and the head position is updated.
// offset < 0
const int offset = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);As I wrote above, a negative offset is allowed, it just has to fit into the list.
The loc identifies the position in the list to insert at. It is not allowed to be negative, otherwise it is out of bounds memory access.
int loc = (m_iHead + offset + m_iSize) % m_iSize;There was a problem hiding this comment.
Ok, I understand that negative loc within the bounds is allowed, but I don't think it's correct to reject the whole report because of that. It should reject it only if the upper bound is behind the seqstart, and if it's not, the loss record should be accepted with the lower bound adjusted.
There was a problem hiding this comment.
Added check for upper sequence.
Work in progress.
Probably
CSndLossList::insert(..)should return-1on failure, but then it needs to be properly handled inCUDT.Related to #1000.
Suggestion by @ethouris